-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: made API token validation async! #96
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
services/api_key.py (1)
Line range hint
51-71
: Improve the validate method implementation.A few suggestions to enhance this method:
- The boolean conversion can be simplified
- Consider adding return type hints in the docstring
Here's a cleaner implementation:
async def validate(self, api_key: str) -> bool: """ check if the api key is available in mongodb or not Parameters ------------ api_key : str the provided key to check in db Returns --------- valid : bool if the key was available in mongo collection, then return True else, the token is not valid and return False + rtype: bool """ document = self.client[self.db][self.tokens_collection].find_one( {"token": api_key} ) - return True if document else False + return bool(document)tests/integration/test_validate_token.py (3)
7-13
: Consider making MongoDB operations async in setupWhile the transition to async testing is good, the MongoDB client initialization could be made async for consistency and to prevent potential blocking operations.
Consider updating the setup:
async def asyncSetUp(self) -> None: """ Set up test case with a test database """ - self.client = MongoSingleton.get_instance().get_client() + self.client = await MongoSingleton.get_instance().get_async_client() self.validator = ValidateAPIKey()
Line range hint
46-66
: Refactor duplicate test data setupThe test data insertion is duplicated across test methods. Consider extracting it to a helper method for better maintainability.
Consider adding a helper method:
async def _insert_test_tokens(self, tokens): """Helper method to insert test tokens Args: tokens (list): List of token documents to insert """ try: await self.client[self.validator.db][self.validator.tokens_collection].insert_many(tokens) except Exception as e: print(f"Failed to insert test tokens: {e}") raiseThen update the test methods to use it:
await self._insert_test_tokens([ {"id": 1, "token": "1111", "options": {}}, {"id": 2, "token": "2222", "options": {}}, {"id": 3, "token": "3333", "options": {}} ])Also applies to: 77-96
102-108
: Consider adding more edge casesWhile testing empty API key is good, consider adding more edge cases for comprehensive validation testing.
Consider adding tests for:
- API key with only whitespace
- API key with special characters
- Very long API key
- Non-string API key types
Example:
async def test_validation_with_whitespace_api_key(self): """Test validation with whitespace API key""" valid = await self.validator.validate(" ") self.assertEqual(valid, False) async def test_validation_with_special_chars(self): """Test validation with special characters in API key""" valid = await self.validator.validate("!@#$%^&*()") self.assertEqual(valid, False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
services/api_key.py
(2 hunks)tests/integration/test_validate_token.py
(3 hunks)
🔇 Additional comments (2)
services/api_key.py (2)
13-30
: LGTM! Well-documented function.
The docstring follows NumPy format and provides clear documentation of parameters, exceptions, and return values.
Line range hint 1-71
: Verify async implementation in dependent modules.
The transition to async validation might require updates in modules that call these methods.
Let's check for any potential callers that need to be updated:
✅ Verification successful
Async implementation verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to get_api_key or ValidateAPIKey.validate
rg -l "get_api_key|ValidateAPIKey.*validate" --type py
# Search for potential FastAPI dependencies using this function
rg -l "Depends.*get_api_key" --type py
Length of output: 117
+ wrong dependency was being applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
routers/http.py (1)
Issue Found:
get_api_key
is not defined as async inservices/api_key.py
.The endpoint is updated to use
get_api_key
as an async dependency, which may cause runtime issues.🔗 Analysis chain
Line range hint
20-37
: LGTM: Endpoint updated to use async API key validation.The dependency change aligns with the PR objective, and the endpoint maintains proper async/await patterns.
Let's verify the async validation is properly integrated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the async validation implementation # Check if the get_api_key function is properly async ast-grep --pattern 'async def get_api_key($$$)' # Check for any remaining sync validation calls rg -l 'api_key_header'Length of output: 233
Script:
#!/bin/bash # Description: Verify if get_api_key is defined as async in services/api_key.py ast-grep --pattern 'async def get_api_key($$)' services/api_key.pyLength of output: 68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
routers/http.py
(3 hunks)
🔇 Additional comments (2)
routers/http.py (2)
7-7
: LGTM: Import added for new dependency.
The import is correctly placed and necessary for the updated dependency injection.
Line range hint 40-61
: Consider making persistence operations async.
While the API key validation is now async, the persist_http
operation is still synchronous and could block the endpoint. Consider updating the PersistPayload
class to support async operations.
Example refactor:
- persister = PersistPayload()
- persister.persist_http(http_payload, update=True)
+ persister = PersistPayload()
+ await persister.persist_http_async(http_payload, update=True)
Let's check the persistence implementation:
#!/bin/bash
# Description: Check if PersistPayload has async support
# Look for async methods in PersistPayload
ast-grep --pattern 'class PersistPayload {
$$$
async def $_($$) {
$$$
}
$$$
}'
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests